Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

option_if_let_else - distinguish pure from impure else expressions #5937

Merged
merged 2 commits into from
Sep 16, 2020
Merged

option_if_let_else - distinguish pure from impure else expressions #5937

merged 2 commits into from
Sep 16, 2020

Conversation

tnielens
Copy link
Contributor

Addresses partially #5821.

changelog: improve the lint option_if_let_else. Suggest map_or or map_or_else based on the else expression purity.

@rust-highfive
Copy link

r? @matthiaskrgr

(rust_highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties label Aug 21, 2020
@tnielens tnielens force-pushed the option_if_let_else branch 2 times, most recently from 5fd410e to 56cea57 Compare August 21, 2020 23:27
@ghost
Copy link

ghost commented Aug 23, 2020

I like clippy would be more consistent overall if this lint used the logic in unnecessary_lazy_evaluations. You could also reuse that code.

@tnielens
Copy link
Contributor Author

I like clippy would be more consistent overall if this lint used the logic in unnecessary_lazy_evaluations. You could also reuse that code.

I'll check unnecessary_lazy_evaluations and try to factor common code out.

@matthiaskrgr matthiaskrgr added S-waiting-on-author Status: This is awaiting some action from the author. (Use `@rustbot ready` to update this status) and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties labels Aug 28, 2020
@tnielens tnielens force-pushed the option_if_let_else branch from cd23b40 to 726c15b Compare August 28, 2020 17:29
@tnielens
Copy link
Contributor Author

The refactoring ended up being quite large. Don't hesitate if you find some changes unnecessary or if you prefer using different naming.

@tnielens tnielens force-pushed the option_if_let_else branch from 726c15b to 0ad345d Compare August 28, 2020 17:35
flip1995
flip1995 reviewed Aug 31, 2020
View reviewed changes
Copy link
Member

@flip1995 flip1995 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The refactoring looks great to me!

clippy_lints/src/utils/eager_or_lazy.rs Outdated Show resolved Hide resolved
tests/ui/unnecessary_lazy_eval.rs Outdated Show resolved Hide resolved
flip1995
flip1995 reviewed Sep 15, 2020
View reviewed changes
clippy_lints/src/methods/bind_instead_of_map.rs Outdated Show resolved Hide resolved
clippy_lints/src/methods/mod.rs Outdated Show resolved Hide resolved
flip1995
flip1995 reviewed Sep 15, 2020
View reviewed changes
Copy link
Member

@flip1995 flip1995 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! One NIT left.

For future contributions, it would be great if you could submit multiple commits for changes this big and then squash them right before merging. This makes reviewing way easier.

tests/ui/unnecessary_lazy_eval.rs Show resolved Hide resolved
flip1995
flip1995 reviewed Sep 16, 2020
View reviewed changes
Copy link
Member

@flip1995 flip1995 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Everything else LGTM now

tests/ui/unnecessary_lazy_eval.rs Outdated
Comment on lines 110 to 112
// should lint, bind_instead_of_map doesn't apply
let _: Result<usize, usize> = res.and_then(|x| Err(x));
let _: Result<usize, usize> = res.or_else(|err| Ok(err));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These two cases aren't linted currently. Is the comment just 2 lines too early or is this a FN?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You're right, the comment is misleading. These are linted neither by bind_instead_of_map nor unnecessary_lazy_eval (because the closure argument is used). I'll make that clear in the comment.

@flip1995 flip1995 mentioned this pull request Sep 16, 2020
Closed
@flip1995
Copy link
Member

Thanks for all the work on this!

@bors r+

@bors
Copy link
Contributor

bors commented Sep 16, 2020

📌 Commit 79da747 has been approved by flip1995

@bors
Copy link
Contributor

bors commented Sep 16, 2020

⌛ Testing commit 79da747 with merge 06f1902...

@bors
Copy link
Contributor

bors commented Sep 16, 2020

☀️ Test successful - checks-action_dev_test, checks-action_remark_test, checks-action_test
Approved by: flip1995
Pushing 06f1902 to master...

@bors bors merged commit 06f1902 into rust-lang:master Sep 16, 2020
@bors bors mentioned this pull request Sep 16, 2020
Merged
@tnielens
Copy link
Contributor Author

Thanks for the many review iterations! @flip1995

@Arnavion Arnavion mentioned this pull request Feb 13, 2021
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@flip1995 flip1995 flip1995 left review comments

@matthiaskrgr matthiaskrgr matthiaskrgr left review comments

Assignees

@matthiaskrgr matthiaskrgr

Labels
S-waiting-on-author Status: This is awaiting some action from the author. (Use `@rustbot ready` to update this status)
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@tnielens @rust-highfive @flip1995 @bors @matthiaskrgr